fix(server): request approval should publish the exact version #1291
Conversation
WalkthroughThe changes in this pull request enhance the GraphQL API for item-related operations by modifying existing mutations and queries to include a Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
server/e2e/gql_porject_test.go (2)
Line range hint 1-1: Fix typo in filename: "porject" should be "project"
The filename contains a typo: gql_porject_test.go should be gql_project_test.go. This should be corrected to maintain code quality and searchability.
50-89: Implementation looks good, but consider adding input validation
The GraphQL mutation and HTTP request handling are well-structured and consistent with the existing patterns. However, consider adding input validation for the parameters before constructing the request.
Add parameter validation:
func updateProject(e *httpexpect.Expect, pID, name, desc, alias, publicationScope string, publicAssets bool) (string, *httpexpect.Value) {
+ if e == nil {
+ panic("httpexpect instance is required")
+ }
+ if pID == "" {
+ panic("project ID is required")
+ }
+ if name == "" {
+ panic("name is required")
+ }
requestBody := GraphQLRequest{server/e2e/gql_model_test.go (1)
Line range hint 339-345: Consider enhancing test coverage for the public visibility feature.
The test only validates the basic update flow with public=false. Consider adding test cases to:
- Verify the behavior with
public=true - Assert the
publicfield value in the response - Test any specific business rules or constraints related to public visibility changes
Example enhancement:
func TestUpdateModel(t *testing.T) {
e := StartServer(t, &app.Config{}, true, baseSeederUser)
pId, _ := createProject(e, wId.String(), "test", "test", "test-2")
mId, _ := createModel(e, pId, "test", "test", "test-2")
- res := updateModel(e, mId, lo.ToPtr("updated name"), lo.ToPtr("updated desc"), lo.ToPtr("updated_key"), false)
+ // Test private model update
+ res := updateModel(e, mId, lo.ToPtr("updated name"), lo.ToPtr("updated desc"), lo.ToPtr("updated_key"), false)
res.Object().
Value("data").Object().
Value("updateModel").Object().
Value("model").Object().
HasValue("name", "updated name").
HasValue("description", "updated desc").
- HasValue("key", "updated_key")
+ HasValue("key", "updated_key").
+ HasValue("public", false)
+
+ // Test public model update
+ res = updateModel(e, mId, lo.ToPtr("public name"), lo.ToPtr("public desc"), lo.ToPtr("public_key"), true)
+ res.Object().
+ Value("data").Object().
+ Value("updateModel").Object().
+ Value("model").Object().
+ HasValue("name", "public name").
+ HasValue("description", "public desc").
+ HasValue("key", "public_key").
+ HasValue("public", true)
}server/e2e/gql_request_test.go (2)
319-355: Consider adding version number validation.
While the version management flow is well tested, consider adding explicit assertions to verify that version numbers are incremented correctly:
ver1 := i1.Path("$.data.createItem.item.version").Raw().(string)
+// Verify version format and sequence
+if len(ver1) == 0 {
+ t.Errorf("Expected non-empty version for initial creation")
+}
ver2 := i1.Path("$.data.updateItem.item.version").Raw().(string)
+if ver2 == ver1 {
+ t.Errorf("Expected version to change after update")
+}301-425: Overall well-structured test with room for enhancement.
The TestRequestFlow function provides excellent coverage of the request approval workflow. To make it even more robust, consider:
- Extracting common setup code into helper functions
- Adding systematic status transition testing
- Including error case coverage
- Adding version number validation
The current implementation is approved, but these enhancements would improve test maintainability and coverage.
server/internal/usecase/interactor/item.go (1)
Line range hint 687-726: Fix event generation to use the latest item version.
The event is being generated with the old item version (itv) instead of the newly fetched version (itm). This could lead to inconsistencies in event data. Consider updating the WebhookObject to use the latest version:
WebhookObject: item.ItemModelSchema{
- Item: itv,
+ Item: itm.Value(),
Model: m,
Schema: s,
GroupSchemas: groupSchemas,
ReferencedItems: refItems,
- Changes: item.CompareFields(itv.Fields(), oldFields),
+ Changes: item.CompareFields(itm.Value().Fields(), oldFields),
},server/e2e/gql_item_test.go (1)
Line range hint 1-1000: Consider adding explicit version handling tests.
While the test suite is comprehensive for CRUD operations and search functionality, consider adding explicit test cases for version handling during request approval flow, which is the main objective of this PR.
Example scenarios to test:
- Version changes during item updates
- Version validation during request approval
- Error cases for version mismatch
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- server/e2e/gql_item_test.go (1 hunks)
- server/e2e/gql_model_test.go (3 hunks)
- server/e2e/gql_porject_test.go (1 hunks)
- server/e2e/gql_request_test.go (1 hunks)
- server/internal/usecase/interactor/item.go (1 hunks)
- server/internal/usecase/interactor/request.go (1 hunks)
🔇 Additional comments (10)
server/e2e/gql_porject_test.go (1)
50-89: Clarify relationship to PR objective
The PR's objective is to fix request approval for publishing exact versions, but this function focuses on project updates and publication settings. Could you clarify how this change relates to the version publishing functionality?
Let's check for related version handling code:
✅ Verification successful
Let me gather more context about the relationship between project publication and version publishing.
Let me gather more specific information about the request approval and project publication relationship.
Based on the gathered information, I can now generate the final response:
Project publication settings are part of request approval flow
The project publication settings are indeed related to the version publishing functionality. When a request is approved, it publishes the version with respect to the project's publication scope (PUBLIC/LIMITED/PRIVATE). The test files show this relationship where project publication settings are configured before testing request approvals, and the version status changes to "PUBLIC" after approval.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version-related mutations and approval logic
rg -A 5 'mutation.*[Vv]ersion|approve.*version|publish.*version' --type go
Length of output: 1015
Script:
#!/bin/bash
# Search for project publication and version publishing relationship
rg -A 10 'publication.*scope|publish.*version' --type go
# Also check for request approval related code
rg -A 10 'approve.*request|handle.*request' --type go
Length of output: 68187
Script:
#!/bin/bash
# Search for request approval and project publication related code in test files
rg -A 10 'TestRequest.*|test.*request.*approve' --type go server/e2e/
# Also check for project publication scope in implementation files
rg -A 10 'ProjectPublicationScope|publication.*scope' --type go server/internal/
Length of output: 18629
server/e2e/gql_model_test.go (2)
Line range hint 47-67: LGTM! Clean implementation of the public visibility parameter.
The function signature and GraphQL mutation are properly updated to handle the new public parameter, maintaining consistency with the existing test patterns.
Line range hint 47-67: Verify alignment with PR objectives.
The PR title mentions "request approval should publish the exact version", but the changes focus on adding public visibility control. Could you clarify how these changes relate to version publishing during request approval?
Let's check for related version handling code:
Also applies to: 339-345
server/internal/usecase/interactor/request.go (1)
282-287: Ensure version fallback doesn't mask underlying issues.
The changes correctly implement exact version publishing with a fallback mechanism. However, a few considerations:
- The fallback to Latest version, while maintaining backward compatibility, might mask underlying issues where
distis unexpectedly nil. - There's a potential race condition if the Latest version changes between the nil check and the UpdateRef call.
Let's verify the impact and usage patterns:
Consider these improvements:
- Add logging when falling back to Latest version to track occurrences
- Consider deprecating the fallback behavior in future versions
- Add version comparison checks to prevent accidental downgrades
Would you like me to propose a more detailed implementation addressing these points?
✅ Verification successful
The search results show that UpdateRef is consistently used across the codebase with similar patterns, particularly in the item interactor and tests. The fallback to Latest version is also used in other parts of the codebase, notably in item.go. This indicates the fallback behavior is an established pattern, not an anomaly.
Let me verify one more aspect about version handling:
Based on the comprehensive search results, I can now provide a final response:
The fallback to Latest version is a well-established pattern and is safe to use
The code changes are correct and align with the existing patterns in the codebase:
- The fallback to
version.Latestis consistently used across the codebase, particularly in item management and version control. - The potential race condition concern is mitigated by the atomic nature of
UpdateRefoperations, as evidenced by the MongoDB implementation. - The nil check for
distis a valid backward compatibility measure, as similar patterns are used in other parts of the codebase (e.g., initem.goand test files).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns where item pointers are set to understand potential nil cases
ast-grep --pattern 'SetPointer($version)'
# Look for other version reference updates to ensure consistency
rg 'UpdateRef.*version\.' -A 3
Length of output: 3642
Script:
#!/bin/bash
# Check version pointer and reference handling patterns
ast-grep --pattern 'Pointer().Ref()'
# Look for version comparison logic
rg 'version\.Compare|version\.Latest' -A 2
Length of output: 19983
server/e2e/gql_request_test.go (1)
301-318: LGTM! Well-structured test setup with clear documentation.
The test setup is comprehensive and properly configures a public project and model with appropriate visibility settings.
server/internal/usecase/interactor/item.go (2)
681-686: LGTM! Important fix to ensure version consistency.
The added re-fetch operation is crucial as it ensures that the exact version of the item after the save operation is returned to the caller. This prevents potential version inconsistencies and aligns with the PR's objective of publishing exact versions.
681-685: Consider transaction rollback on re-fetch failure.
While the re-fetch operation is crucial, its failure could leave the system in an inconsistent state. Consider whether a transaction rollback would be appropriate if the re-fetch fails, as the caller would otherwise have no way to get the correct version of the saved item.
server/e2e/gql_item_test.go (3)
303-303: LGTM: Version field addition aligns with PR objective.
The addition of the version field to the GraphQL mutation response is consistent with the requirement to handle exact versions for request approvals.
Line range hint 1-1000: LGTM: Well-structured and comprehensive test suite.
The test suite demonstrates good practices:
- Modular helper functions
- Clear test scenarios
- Comprehensive assertions
- Good error case coverage
Line range hint 1-1000: Verify request approval implementation.
Let's verify the complete implementation of request approval with version handling.
✅ Verification successful
Let me gather more information about the version handling in request approval by checking the implementation details.
Let me check the GraphQL schema in a different location and look for version handling in request approval flow.
Let me check the request GraphQL schema and the complete request approval implementation.
Based on the gathered information, I can now provide a final response regarding the version handling in request approval.
Version handling is properly implemented in request approval flow
The implementation correctly handles versions through:
RequestItemtype in GraphQL schema includesversion: Stringfield- Version information is preserved in request items during approval
- Request state transitions are properly managed (DRAFT -> WAITING -> APPROVED)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for request approval implementation with version handling
# Test: Search for request approval related files and functions
echo "Searching for request approval related files..."
rg -l "request.*approval|approve.*request" --type go
# Test: Search for version handling in request approval
echo "Searching for version handling in request approval..."
rg -A 5 "func.*[Aa]pprove.*Request" --type go
Length of output: 7302
Script:
#!/bin/bash
# Check request approval implementation details
# Look for version handling in request implementation
echo "Checking request implementation..."
rg -A 10 "type Request struct" server/internal/usecase/interactor/request.go
# Look for version field in request model
echo "Checking request model..."
rg -A 10 "type Request struct" server/pkg/request/request.go
# Look for version in GraphQL schema
echo "Checking GraphQL schema..."
rg "type Request" server/internal/adapter/gql/schema/*.graphql
Length of output: 967
Script:
#!/bin/bash
# Check request approval and version handling
# Look for GraphQL schema files
echo "Finding GraphQL schema files..."
fd "\.graphql$"
# Check request approval implementation details
echo "Checking request approval implementation..."
rg -A 15 "func \(r Request\) Approve" server/internal/usecase/interactor/request.go
# Check request state handling
echo "Checking request state handling..."
rg -A 5 "type State" server/pkg/request/request.go
Length of output: 1680
Script:
#!/bin/bash
# Check request schema and approval implementation
# Check request GraphQL schema
echo "Checking request GraphQL schema..."
cat server/schemas/request.graphql
# Check complete request approval implementation
echo "Checking complete request approval implementation..."
rg -A 30 "func \(r Request\) Approve" server/internal/usecase/interactor/request.go
# Check request state transitions
echo "Checking request state transitions..."
rg "Approve|state" server/pkg/request/request.go
Length of output: 3528
| // 7- approve request | ||
| res = approveRequest(e, rid) | ||
| req = res.Path("$.data.approveRequest.request").Object() | ||
| req.Value("id").IsEqual(rid) | ||
| req.Value("state").IsEqual("APPROVED") | ||
|
|
||
| // check item and its status | ||
| _, itm := getItem(e, iid1) | ||
| itm.Path("$.data.node").Object().Value("version").IsEqual(ver3) | ||
| itm.Path("$.data.node").Object().Value("status").IsEqual("PUBLIC_DRAFT") | ||
|
|
||
| // check public item: should return version 2 | ||
| res = e.GET("/api/p/{project}/{model}", "test-1", "test-1"). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON() | ||
| res.IsEqual(map[string]any{ | ||
| "results": []map[string]any{ | ||
| { | ||
| "id": iid1, | ||
| "text": "v2", | ||
| }, | ||
| }, | ||
| "totalCount": 1, | ||
| "hasMore": false, | ||
| "limit": 50, | ||
| "offset": 0, | ||
| "page": 1, | ||
| }) | ||
|
|
||
| // 8- create request with version 3 | ||
| res = createRequest(e, pId, "test", lo.ToPtr("test"), lo.ToPtr("WAITING"), []string{uId1.String()}, []any{map[string]any{"itemId": iid1, "version": ver3}}) | ||
| req = res.Path("$.data.createRequest.request").Object() | ||
| rid = req.Value("id").String().Raw() | ||
|
|
||
| // check item and its status | ||
| _, itm = getItem(e, iid1) | ||
| itm.Path("$.data.node").Object().Value("version").IsEqual(ver3) | ||
| itm.Path("$.data.node").Object().Value("status").IsEqual("PUBLIC_REVIEW") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using table-driven tests for status transitions.
The status transition testing could be more comprehensive using a table-driven approach to test all possible state transitions:
// Add before the test function
var statusTransitionTests = []struct {
initialStatus string
action string
expectedStatus string
}{
{"DRAFT", "approve", "PUBLIC_DRAFT"},
{"PUBLIC_DRAFT", "request", "PUBLIC_REVIEW"},
{"PUBLIC_REVIEW", "approve", "PUBLIC"},
}
// Then use in test:
for _, tt := range statusTransitionTests {
t.Run(fmt.Sprintf("%s->%s", tt.initialStatus, tt.expectedStatus), func(t *testing.T) {
// Test status transition
})
}| // 9- approve request | ||
| res = approveRequest(e, rid) | ||
| req = res.Path("$.data.approveRequest.request").Object() | ||
| req.Value("id").IsEqual(rid) | ||
| req.Value("state").IsEqual("APPROVED") | ||
|
|
||
| // check item and its status | ||
| _, itm = getItem(e, iid1) | ||
| itm.Path("$.data.node").Object().Value("version").IsEqual(ver3) | ||
| itm.Path("$.data.node").Object().Value("status").IsEqual("PUBLIC") | ||
|
|
||
| // check public item: should return version 3 | ||
| res = e.GET("/api/p/{project}/{model}", "test-1", "test-1"). | ||
| Expect(). | ||
| Status(http.StatusOK). | ||
| JSON() | ||
| res.IsEqual(map[string]any{ | ||
| "results": []map[string]any{ | ||
| { | ||
| "id": iid1, | ||
| "text": "v3", | ||
| }, | ||
| }, | ||
| "totalCount": 1, | ||
| "hasMore": false, | ||
| "limit": 50, | ||
| "offset": 0, | ||
| "page": 1, | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the gathered information, I can now generate the final response.
Add test cases for request approval error scenarios
The codebase already has error handling for invalid request states, but test coverage is missing. Add test cases for:
- Approving a request with "DRAFT" or "CLOSED" state (only "WAITING" state can be approved)
- Approving a request when the operator is not a user
- Approving a request that fails during save operation
Example:
t.Run("approve_non_waiting_request", func(t *testing.T) {
req := createRequest(e, pId, "test", lo.ToPtr("test"), lo.ToPtr("DRAFT"),
[]string{uId1.String()},
[]any{map[string]any{"itemId": iid1, "version": "1.0"}})
res := approveRequest(e, req.Path("$.data.createRequest.request.id").String().Raw())
res.Path("$.errors").Array().NotEmpty()
})🔗 Analysis chain
Add error case testing for request approval flow.
While the happy path is well tested, consider adding test cases for error scenarios:
Consider adding test cases for:
- Approving an already approved request
- Approving a request with an invalid version
- Approving a request for a non-existent item
- Concurrent request approvals for the same item
Example:
t.Run("approve_invalid_version", func(t *testing.T) {
// Setup request with invalid version
invalidVer := "invalid-version"
res := createRequest(e, pId, "test", lo.ToPtr("test"), lo.ToPtr("WAITING"),
[]string{uId1.String()},
[]any{map[string]any{"itemId": iid1, "version": invalidVer}})
// Verify error response
res.Path("$.errors").Array().NotEmpty()
})Let me gather more information about the request approval flow to better understand the error cases.
Let me search for more specific information about request approval handling.
Let me search for request state definitions and error handling in the usecase layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find potential error cases in the codebase
rg -A 3 "errors\.New|fmt\.Errorf" server/internal/usecase/interactor/
Length of output: 7822
Script:
#!/bin/bash
# Find request approval related code and error handling
ast-grep --pattern 'func $_ approveRequest($$$) {
$$$
}'
# Also check for request state related code
rg -A 5 "request.*state" server/internal/usecase/interactor/request.go
Length of output: 335
Script:
#!/bin/bash
# Search for request approval related code in the repository
rg -A 10 "ApproveRequest|approveRequest" server/
# Look for request state constants or enums
rg -A 5 "type.*State.*=.*string|iota" server/
Length of output: 23433
Script:
#!/bin/bash
# Search for request state definitions
rg -A 5 "const.*State|type.*State|iota.*State" server/internal/
# Search for request approval related error handling in usecase
rg -A 5 "func.*Approve|error.*request" server/internal/usecase/
Length of output: 2523
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- server/internal/usecase/interactor/item_test.go (1 hunks)
🔇 Additional comments (1)
server/internal/usecase/interactor/item_test.go (1)
995-1006: LGTM! Proper test setup with error handling.
The test setup is thorough and follows a logical order with proper error handling for each database operation.
|
|
||
| ctx := context.Background() | ||
| db := memory.New() | ||
| err := db.Project.Save(ctx, prj) | ||
| assert.NoError(t, err) | ||
| err = db.Schema.Save(ctx, s) | ||
| assert.NoError(t, err) | ||
| err = db.Model.Save(ctx, m) | ||
| assert.NoError(t, err) | ||
| err = db.Item.Save(ctx, i) | ||
| assert.NoError(t, err) | ||
|
|
||
| vi, err := db.Item.FindByID(ctx, i.ID(), nil) | ||
| assert.NoError(t, err) | ||
| ri, _ := request.NewItem(i.ID(), lo.ToPtr(vi.Version().String())) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring test setup for better maintainability.
The test setup could be improved by:
- Extracting the database setup into a helper function to reduce boilerplate.
- Using table-driven tests to cover different workflow scenarios.
+func setupTestData(t *testing.T, ctx context.Context, db *memory.Container) (*project.Project, *schema.Schema, *model.Model, *item.Item) {
+ prj := project.New().NewID().Workspace(wid).MustBuild()
+ s := schema.New().NewID().Workspace(accountdomain.NewWorkspaceID()).Project(prj.ID()).MustBuild()
+ m := model.New().NewID().Project(prj.ID()).Schema(s.ID()).RandomKey().MustBuild()
+ i := item.New().NewID().Schema(s.ID()).Model(m.ID()).Project(prj.ID()).Thread(id.NewThreadID()).MustBuild()
+
+ assert.NoError(t, db.Project.Save(ctx, prj))
+ assert.NoError(t, db.Schema.Save(ctx, s))
+ assert.NoError(t, db.Model.Save(ctx, m))
+ assert.NoError(t, db.Item.Save(ctx, i))
+
+ return prj, s, m, i
+}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
server/internal/usecase/interactor/request_test.go (3)
365-367: Remove or implement the commented error handling code.
The commented-out error handling code should either be implemented or removed to maintain code clarity.
368-375: Standardize error handling pattern.
Consider standardizing the error handling pattern. Currently, some errors are handled directly with assert.NoError while others are stored in a variable first. For consistency, prefer direct assertion:
-err := db.Project.Save(ctx, prj)
-assert.NoError(t, err)
+assert.NoError(t, db.Project.Save(ctx, prj))355-355: Address TODO: Add error test cases.
The test currently only covers the success path. Consider adding test cases for:
- Invalid request ID
- Unauthorized user (non-reviewer)
- Invalid item version
- Database errors during approval
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- server/internal/usecase/interactor/request_test.go (1 hunks)
🔇 Additional comments (2)
server/internal/usecase/interactor/request_test.go (2)
356-362: LGTM: Well-structured test data initialization.
The test data setup follows a clear dependency chain and properly establishes the necessary entities for testing request approval.
377-387: LGTM: Explicit version handling ensures correct version publication.
The test now properly verifies that the exact version is published during request approval by:
- Explicitly retrieving the item version from the database
- Creating the request item with the specific version
- Verifying the published version after approval
This change aligns with the PR objective of ensuring exact versions are published during request approval.
Summary by CodeRabbit
New Features
versionfield in item creation, retrieval, updating, and deletion functionalities.Bug Fixes
Tests
Documentation